Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix benchmark writer #9626

Merged
2 commits merged into from
Aug 27, 2021
Merged

Fix benchmark writer #9626

2 commits merged into from
Aug 27, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Aug 25, 2021

when running benchmarks the function map_result make some invalid assumption about the input. (or the caller doesn't correctly order the input).
actually it happens in this PR paritytech/polkadot#3511 that benchmark result are ordered like this:

pallet_collective, council_instance, first_method,
pallet_collective, technical_committee, first_method,
pallet_collective, council_instance, second_method,
pallet_collective, technical_committee, second_method,
....

So the map_result will remove all the methods except the last one.

This refactor make the map_result function more general and simpler. And this should fix the issue with running the benchmark for multiple pallets.

Needed for paritytech/polkadot#3511

@gui1117 gui1117 added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 25, 2021
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feeling pretty dumb not to write it this way to begin with

@paritytech paritytech deleted a comment from parity-benchapp bot Aug 27, 2021
@shawntabrizi shawntabrizi self-requested a review August 27, 2021 13:18
@gui1117
Copy link
Contributor Author

gui1117 commented Aug 27, 2021

bot merge

@ghost
Copy link

ghost commented Aug 27, 2021

Trying merge.

@ghost ghost merged commit f7b6dca into master Aug 27, 2021
@ghost ghost deleted the gui-fix-benchmark-writer branch August 27, 2021 13:43
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants